Stepper accessibility fixes #4571
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-stepper-a11y.surge.sh |
|
Size Change: +103 B (+0.02%) Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
5ace489
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1aa8bb17.orbit.pages.dev |
| Branch Preview URL: | https://sarka-stepper-a11y.orbit.pages.dev |
sarkaaa
commented
Jan 9, 2025
sarkaaa
commented
Jan 9, 2025
packages/orbit-components/src/Stepper/StepperStateless/index.tsx
Outdated
Show resolved
Hide resolved
sarkaaa
commented
Jan 9, 2025
| icons={iconStyles} | ||
| title={titleIncrement} |
There was a problem hiding this comment.
I was thinking of changing this prop name from title to ariaLabel in ButtonPrimitive and making it more specific, however, this would be (probably) BC. I searched where this prop is used and I've found it's used here.
I'm not sure if it makes sense to change this prop name, if it could be included within this PR or in a separate one.
There was a problem hiding this comment.
So your suggestion is to keep prop names in Stepper as titleIncrement and titleDecrement and change the prop on the ButtonPrimitive from title to ariaLabel so the usage would be ariaLabel={titleIncrement}?
But anyway, I think it's out of scope and this should be tackled when making Button/ButtonPrimitive accessible.
There was a problem hiding this comment.
No, that wouldn't make sense. :D My suggestion was to rename aria attribute in ButtonPrimitive (probably in a different PR) and also change titleIncrement (titleDecrement). I kept titleDecrement (titleIncrement) because of attribute naming in ButtonPrimitive.
As you said, I also think it's out of the scope of this PR, that's the reason why I didn't touch ButtonPrimitive at all.
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
aa7965f to
b925e95
Compare
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
6640b0d to
3f6ed58
Compare
There was a problem hiding this comment.
I didn't leave comments for the aria-live related stuff as I think we shouldn't be adding it, but let's discuss that in the comment I posted.
Overall, by looking at the component, I am thinking if maybe we should connect the individual subcomponents so it's clear that they are connected. Looking at the usage, I found this example:
I guess we could add aria-labelledby to the Stepper so it's possible to connect these categories to the individual Steppers? You added it to the documentation, but you didn't added it to the component.
Also, maybe we could add aria-controls internally to the buttons to connect them with the displayed text? You again added it to the documentation and not the component, but I think this one could be added only internally to buttons/input and not as a prop to Stepper component. Maybe aria-controls is not the best solution for this, maybe you have more information and suggestions.
WDYT?
packages/orbit-components/src/Stepper/StepperStateless/index.tsx
Outdated
Show resolved
Hide resolved
| icons={iconStyles} | ||
| title={titleIncrement} |
There was a problem hiding this comment.
So your suggestion is to keep prop names in Stepper as titleIncrement and titleDecrement and change the prop on the ButtonPrimitive from title to ariaLabel so the usage would be ariaLabel={titleIncrement}?
But anyway, I think it's out of scope and this should be tackled when making Button/ButtonPrimitive accessible.
|
|
||
| The Stepper component has been designed with accessibility in mind. It can be used with keyboard navigation and includes properties that enhance the experience for users of assistive technologies. | ||
|
|
||
| Although the `ariaLabel` and `ariaLive` props are optional for the Stepper (StepperStateless) component itself, it is recommended to fill them in to ensure that the component can be perceived by assistive technologies. This ensures that the Stepper (StepperStateless) component is accessible. |
There was a problem hiding this comment.
Why didn't you add the same table as in the Seat Accessibility tab?
There was a problem hiding this comment.
there's no specific reason why I added it as a section (as it's in some other components). but I agree we can have accessibility things only in accessibility tags.
docs/src/documentation/03-components/07-interaction/stepper/03-accessibility.mdx
Outdated
Show resolved
Hide resolved
| /> | ||
| ``` | ||
|
|
||
| The screen reader will announce stepper title (`Passengers number`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by screen reader. |
There was a problem hiding this comment.
Is it stepper title? I thought it's title of the input? From "stepper title" I would assume it's the title for the whole component, including the buttons and displayed text.
Will the screen reader first announce "Passengers number" and only then "Remove a passenger" and "Add a passenger"? It won't be "Remove a passenger", then "Passengers number" and then "Add a passenger"?
There was a problem hiding this comment.
Hm, but maybe it should be value title instead of input title? Because we are referring to the number with value name in the docs.
docs/src/documentation/03-components/07-interaction/stepper/03-accessibility.mdx
Show resolved
Hide resolved
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
2 times, most recently
from
ae01432 to
afe75c2
Compare
Adding |
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
4 times, most recently
from
15f0b14 to
de793bf
Compare
|
Storybook staging is available at https://kiwicom-orbit-sarka-stepper-a11y.surge.sh Playroom staging is available at https://kiwicom-orbit-sarka-stepper-a11y.surge.sh/playroom |
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
4 times, most recently
from
fce1b7e to
d2dae24
Compare
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
3 times, most recently
from
dc22ee6 to
f6dd372
Compare
packages/orbit-components/src/Stepper/StepperStateless/index.tsx
Outdated
Show resolved
Hide resolved
| | :--------- | | ||
| | `"small"` | | ||
| | `"normal"` | | ||
| | titleDecrement | `string \| (any => string)` | | Specifies `ariaLabel` property on decrement `Button`. | |
There was a problem hiding this comment.
Hm, I am not sure about this change 🤔 Maybe it could be "Specifies title property on decrement Button, added as aria-label attribute/value."
Or, at least, wouldn't it make more sense to say "...aria-label attribute/value... "? As there is no prop called ariaLabel on Button.
| /> | ||
| ``` | ||
|
|
||
| The screen reader will announce stepper title (`Passengers number`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by screen reader. |
There was a problem hiding this comment.
Hm, but maybe it should be value title instead of input title? Because we are referring to the number with value name in the docs.
docs/src/documentation/03-components/07-interaction/stepper/03-accessibility.mdx
Outdated
Show resolved
Hide resolved
docs/src/documentation/03-components/07-interaction/stepper/03-accessibility.mdx
Show resolved
Hide resolved
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
from
f6dd372 to
72957e0
Compare
| </Stack> | ||
| ``` | ||
|
|
||
| This example includes `ariaLabelledby` prop. In this case, `ariaLabelledBy` prop is prioritized over `ariaLabel`, so the screen reader will announce the value title (`Passengers`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by the screen reader. |
There was a problem hiding this comment.
Suggested change
| This example includes `ariaLabelledby` prop. In this case, `ariaLabelledBy` prop is prioritized over `ariaLabel`, so the screen reader will announce the value title (`Passengers`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by the screen reader. | |
| This example includes `ariaLabelledby` prop. In this case, `ariaLabelledBy` prop is prioritized over `ariaLabelValue`, so the screen reader will announce the value title (`Passengers`) and buttons title (`Add a passenger`, `Remove a passenger`) once they are focused by the screen reader. |
sarkaaa
force-pushed
the
sarka/stepper-a11y
branch
from
72957e0 to
5ace489
Compare
domihustinova
approved these changes
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2195
This PR fixes these a11y violations:
✨
Description by Callstackai
This PR introduces accessibility fixes for the Stepper component, ensuring compliance with a11y standards by adding necessary ARIA attributes.
Diagrams of code changes
sequenceDiagram participant User participant Screen Reader participant Stepper participant Input participant Increment Button participant Decrement Button Note over Stepper: New Accessibility Props Added User->>Stepper: Interacts with component alt Focus on Input Screen Reader->>Input: Reads aria-label from ariaLabelValue Screen Reader->>Input: Or reads label from ariaLabelledBy end alt Focus on Increment Button Screen Reader->>Increment Button: Reads titleIncrement Note right of Increment Button: "Add a passenger" end alt Focus on Decrement Button Screen Reader->>Decrement Button: Reads titleDecrement Note right of Decrement Button: "Remove a passenger" endFiles Changed
This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.mdx,.md. See list of supported languages.